Only pass strings to HttpRequest#setHeader
#709
Conversation
DraganDanicic
commented
Aug 23, 2024
•
DraganDanicic
commented
- Numbers passed as header-values are casted to string type.
- Tests adjusted in a way that expected header values are now strings.
|
Thank you for the PR! All of these should be addressed once we get #683 merged, which moves the code base to TypeScript. While working on it, I also discovered that we call setHeader sometimes with a number, which is not ideal. But we can also get this PR faster merged without waiting on the TypeScript PR. |
lib/upload.js
Outdated
| @@ -378,7 +395,7 @@ class BaseUpload { | |||
| // Add metadata if values have been added | |||
| const metadata = encodeMetadata(this.options.metadata) | |||
| if (metadata !== '') { | |||
| req.setHeader('Upload-Metadata', metadata) | |||
| req.setHeader('Upload-Metadata', BaseUpload.convertToStringHeaderValue(metadata)) | |||
There was a problem hiding this comment.
encodeMetadata always returns a string, so we don't need a conversion here.
lib/upload.js
Outdated
| } else { | ||
| req.setHeader('Upload-Length', this._size) | ||
| req.setHeader('Upload-Length', BaseUpload.convertToStringHeaderValue(this._size)) |
There was a problem hiding this comment.
Most violations come from Upload-Length and Upload-Offset as you already found out. We can just use ${this._size} or similar here to convert the numbers into strings without the additional conversion method.
lib/upload.js
Outdated
| @@ -973,7 +990,7 @@ function openRequest(method, url, options) { | |||
| const headers = options.headers || {} | |||
|
|
|||
| for (const [name, value] of Object.entries(headers)) { | |||
| req.setHeader(name, value) | |||
| req.setHeader(name, BaseUpload.convertToStringHeaderValue(value)) | |||
There was a problem hiding this comment.
I don't think we should magically convert non-strings into strings here, especially since this is technically a breaking change. It's the user responsibility to pass proper values in here that match what the documentation mentions.
We can log a warning if the value is not a string and then convert the warning into an error in the next major release.
lib/upload.js
Outdated
| @@ -155,6 +155,23 @@ class BaseUpload { | |||
| }) | |||
| } | |||
|
|
|||
| static convertToStringHeaderValue(val) { | |||
There was a problem hiding this comment.
The helper method is a good idea, but not really needed. I would like to avoid a large helper method like this that magically converts everything somehow into a string and instead use dedicated methods where we need to convert things into string.
I agree on your points. Shall we then go with explicit conversion at-the-spot (this -> |
Yes, that would be my preferred solution. |
DraganDanicic
changed the title
I have adjusted the code, and it should now meet the requirements. Please, have a look. |
Acconut
changed the title
HttpRequest#setHeader
